Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved build tools (preprocessor & postprocessor) #6189

Merged
merged 3 commits into from
Jul 19, 2015

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Jul 9, 2015

I found many defects in the preprocessor, so I've fixed them (and added unit tests to give confidence that the features work as intended). See the commit messages for a detailled account of what was changed, and why.

In particular, nested preprocessor conditions work as expected now:

//#if FALSE
//#else
//#if CHROME
//// This should be shown, but it was not before the patch.
//#endif
//#endif

The previous can also be shortened with the new #elif command:

//#if FALSE
//#elif CHROME
//// This should be shown, but it was not before the patch.
//#endif

@Rob--W Rob--W added the other label Jul 9, 2015
@Rob--W Rob--W force-pushed the improved-build-tools branch 3 times, most recently from 65e35b6 to 5f6b03e Compare July 13, 2015 22:18
@Snuffleupagus
Copy link
Collaborator

Review progress:

  • Improve getWorkerSrcFiles (builder.js)
  • make.js: Less greedy comment stripping
  • Fix preprocessor: nesting, error & tests

@Snuffleupagus Snuffleupagus self-assigned this Jul 19, 2015
@Snuffleupagus
Copy link
Collaborator

When running the added unit-tests (which are really nice to have, btw), I'm getting the following output:

Assertion failed for undefined-define.js
--------------------------------------------------
EXPECTED:
//Error: Could not evaluate "notdefined" at c:\Users\Jonas\Git\pdfjs\external\builder\fixtures\undefined-define.js:2
//ReferenceError: evalmachine.<anonymous>:1
//notdefined
//^
//notdefined is not defined
--------------------------------------------------
ACTUAL
//Error: Could not evaluate "notdefined" at c:\Users\Jonas\Git\pdfjs\external\builder\fixtures\undefined-define.js:2
//ReferenceError: notdefined is not defined
--------------------------------------------------

All tests completed without errors.

Is this result expected? I'm running the tests on Windows, if that matters.

@Rob--W
Copy link
Member Author

Rob--W commented Jul 19, 2015

@Snuffleupagus The test failure is not expected, what Node version are you using? I'm using 0.12.5.

The exact format of the error message is an implementation detail of Node, so a slightly different error is not too surprising.

@Snuffleupagus
Copy link
Collaborator

I was using version 0.10.26. However, I've just updated to version 0.12.7 and now the tests pass as expected!
If there's a (simple) way to make this more robust with respect to different Node versions, that would be nice, but that is not a requirement as far as I'm concerned.

It took a while to figure out why adding comments in worker_loader.js
caused the build to fail, because getWorkerSrcFiles did not print an
error message when it failed to parse the file. These issues have been
resolved as follows:

- Leading comments are stripped.
- The trailing comma is removed from the array.
- Errors are detected and useful error messages are printed.
The previous regex was too greedy, it stripped a significant amount of
code when I put another file after core/murmurhash3.js. This was caused
by the fact that the license header in murmurhash3.js does not contain
"Mozilla Foundation", so the regex continued to match until my new file
(which had the standard license header containing "Mozilla Foundation").
Features / bug fixes in the preprocessor:

- Add word boundary after regex for preprocessor token matching.
  Previously, when you mistakenly used "#ifdef" instead of "#if", the
  line would be parsed as a preprocessor directive (because "#ifdef"
  starts with "#if"), but without condition (because "def" does not
  start with a space). Consequently, the condition would always be false
  and anything between "#ifdef" and "#endif" would not be included.

- Add validation and error reporting everywhere, to aid debugging.

- Support nested comments (by accounting for the whole stack of
  conditions, instead of only the current one).

- Add #elif preprocessor command. Could be used as follows:
  //#if !FEATURE_ENABLED
  //#error FEATURE_ENABLED must be set
  //#endif

- Add #error preprocessor command.

- Add end-of-line word boundary after "-->" in the comment trimmer.
  Otherwise the pattern would also match "-->" in the middle of a line,
  and incorrectly convert something like "while(i-->0)" to "while(i0)".

Code health:

- Add unit tests for the preprocessor (run external/builder/test.js).

- Fix broken link to MDN (resolved to DXR).

- Refactor to use STATE_* names instead of magic numbers (the original
  meaning of the numbers is preserved, with one exception).

- State 3 has been split in two states, to distinguish between being in
  an #if and #else. This is needed to ensure that #else cannot be
  started without an #if.
@Rob--W
Copy link
Member Author

Rob--W commented Jul 19, 2015

@Snuffleupagus It's possible, by setting displayErrors to false - https://nodejs.org/api/vm.html#vm_vm_runinthiscontext_code_options.

I've updated the PR with all nits.

diff --git a/external/builder/builder.js b/external/builder/builder.js
index cee1700..d3b6ed8 100644
--- a/external/builder/builder.js
+++ b/external/builder/builder.js
@@ -58,10 +58,10 @@ function preprocess(inFilename, outFilename, defines) {
       throw new Error('No JavaScript expression given at ' + loc());
     }
     try {
-      return vm.runInNewContext(code, defines);
+      return vm.runInNewContext(code, defines, {displayErrors: false});
     } catch (e) {
       throw new Error('Could not evaluate "' + code + '" at ' + loc() + '\n' +
-          e.name + ': ' + e.message);
+                      e.name + ': ' + e.message);
     }
   }
   function include(file) {
@@ -125,7 +125,7 @@ function preprocess(inFilename, outFilename, defines) {
             state = evaluateCondition(m[2]) ? STATE_IF_TRUE : STATE_IF_FALSE;
           } else if (state === STATE_ELSE_TRUE || state === STATE_ELSE_FALSE) {
             throw new Error('Found #elif after #else at ' + loc());
-          } else if (state !== STATE_IF_FALSE) {
+          } else {
             throw new Error('Found #elif without matching #if at ' + loc());
           }
           break;
@@ -172,7 +172,7 @@ function preprocess(inFilename, outFilename, defines) {
   }
   if (state !== STATE_NONE || stack.length !== 0) {
     throw new Error('Missing #endif in preprocessor for ' +
-        fs.realpathSync(inFilename));
+                    fs.realpathSync(inFilename));
   }
   if (typeof outFilename !== 'function') {
     fs.writeFileSync(outFilename, out);
@@ -319,7 +319,7 @@ function getWorkerSrcFiles(filePath) {
     files = JSON.parse(files);
   } catch (e) {
     throw new Error('Failed to parse otherFiles in ' + filePath + ' as JSON, ' +
-        e);
+                    e);
   }

   var srcFiles = files.filter(function(name) {
diff --git a/external/builder/fixtures/undefined-define-expected.js b/external/builder/fixtures/undefined-define-expected.js
index a85fcf9..67bfbca 100644
--- a/external/builder/fixtures/undefined-define-expected.js
+++ b/external/builder/fixtures/undefined-define-expected.js
@@ -1,5 +1,2 @@
 //Error: Could not evaluate "notdefined" at __filename:2
-//ReferenceError: evalmachine.<anonymous>:1
-//notdefined
-//^
-//notdefined is not defined
+//ReferenceError: notdefined is not defined
diff --git a/make.js b/make.js
index 208047c..b35b68e 100644
--- a/make.js
+++ b/make.js
@@ -605,9 +605,9 @@ function stripCommentHeaders(content, filename) {
   // Strip out all the vim/license headers.
   var notEndOfComment = '(?:[^*]|\\*(?!/))+';
   var reg = new RegExp(
-      '\n/\\* -\\*- Mode' + notEndOfComment + '\\*/\\s*' +
-      '(?:/\\*' + notEndOfComment + '\\*/\\s*|//(?!#).*\n\\s*)+' +
-      '\'use strict\';', 'g');
+    '\n/\\* -\\*- Mode' + notEndOfComment + '\\*/\\s*' +
+    '(?:/\\*' + notEndOfComment + '\\*/\\s*|//(?!#).*\n\\s*)+' +
+    '\'use strict\';', 'g');
   content = content.replace(reg, '');
   return content;
 }

Snuffleupagus added a commit that referenced this pull request Jul 19, 2015
Improved build tools (preprocessor & postprocessor)
@Snuffleupagus Snuffleupagus merged commit a58393f into mozilla:master Jul 19, 2015
@Snuffleupagus
Copy link
Collaborator

Really good, thanks for the patches!

@timvandermeij
Copy link
Contributor

Nice work on the unit tests, too! It is great that this patch makes the build tools not only more complete, but also testable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants